[DO NOT MERGE] feat(core): add opt-in assertion instrumentation - #6592
[DO NOT MERGE] feat(core): add opt-in assertion instrumentation#6592antonis wants to merge 12 commits into
Conversation
Add an opt-in Metro/Babel transform that rewrites assertion call sites (invariant, assert, warning, console.assert) so a violated assertion reports a non-fatal Sentry event instead of being stripped from release bundles or crashing with a minified message. Adds the captureInvariantViolation runtime API and fixes a related gap in the RN global error handler so an already-captured, re-thrown error is not reported a second time. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📲 Install BuildsAndroid
|
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ecf47a2+dirty | 3835.20 ms | 1222.43 ms | -2612.77 ms |
| 3817909+dirty | 1183.90 ms | 1187.50 ms | 3.60 ms |
| 5ee78d6+dirty | 3847.52 ms | 1224.27 ms | -2623.25 ms |
| 890d145+dirty | 1223.59 ms | 1231.37 ms | 7.78 ms |
| e763471+dirty | 3841.16 ms | 1218.06 ms | -2623.09 ms |
| 3d31fcf+dirty | 3838.09 ms | 1223.46 ms | -2614.63 ms |
| 9210ae6+dirty | 3815.93 ms | 1214.14 ms | -2601.79 ms |
| 26843eb+dirty | 3861.60 ms | 1238.77 ms | -2622.82 ms |
| 7a89652+dirty | 3861.46 ms | 1229.61 ms | -2631.85 ms |
| 9ad4522+dirty | 3850.40 ms | 1222.40 ms | -2628.00 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ecf47a2+dirty | 4.98 MiB | 6.46 MiB | 1.49 MiB |
| 3817909+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| 5ee78d6+dirty | 5.15 MiB | 6.69 MiB | 1.53 MiB |
| 890d145+dirty | 3.38 MiB | 4.77 MiB | 1.38 MiB |
| e763471+dirty | 4.98 MiB | 6.51 MiB | 1.53 MiB |
| 3d31fcf+dirty | 4.98 MiB | 6.56 MiB | 1.58 MiB |
| 9210ae6+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 26843eb+dirty | 4.98 MiB | 6.53 MiB | 1.55 MiB |
| 7a89652+dirty | 5.15 MiB | 6.70 MiB | 1.55 MiB |
| 9ad4522+dirty | 4.98 MiB | 6.56 MiB | 1.58 MiB |
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ad66da3+dirty | 3855.02 ms | 1213.43 ms | -2641.59 ms |
| 7d8c8bd+dirty | 3847.98 ms | 1230.77 ms | -2617.21 ms |
| 4acd4e9+dirty | 3806.73 ms | 1208.48 ms | -2598.25 ms |
| c2e182c+dirty | 3841.76 ms | 1220.76 ms | -2621.01 ms |
| 3d377b5+dirty | 1201.55 ms | 1201.80 ms | 0.25 ms |
| c004dae+dirty | 3857.82 ms | 1224.87 ms | -2632.95 ms |
| a858ac3+dirty | 3850.72 ms | 1226.35 ms | -2624.37 ms |
| 4e0b819+dirty | 3828.96 ms | 1205.64 ms | -2623.32 ms |
| 5c1e987+dirty | 1208.43 ms | 1220.72 ms | 12.29 ms |
| 882f8ae+dirty | 3842.51 ms | 1230.40 ms | -2612.11 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| ad66da3+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| 7d8c8bd+dirty | 5.15 MiB | 6.68 MiB | 1.53 MiB |
| 4acd4e9+dirty | 4.98 MiB | 6.53 MiB | 1.55 MiB |
| c2e182c+dirty | 4.98 MiB | 6.51 MiB | 1.53 MiB |
| 3d377b5+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| c004dae+dirty | 5.15 MiB | 6.67 MiB | 1.51 MiB |
| a858ac3+dirty | 4.98 MiB | 6.51 MiB | 1.53 MiB |
| 4e0b819+dirty | 4.98 MiB | 6.46 MiB | 1.49 MiB |
| 5c1e987+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| 882f8ae+dirty | 5.15 MiB | 6.70 MiB | 1.54 MiB |
Rename the public API from invariant/loudInvariants to an assertions vocabulary: `loudInvariants` config → `captureAssertions`, `captureInvariantViolation` → `captureAssertionViolation`, `InvariantViolationOptions` → `AssertionViolationOptions`, `DEFAULT_INVARIANT_MECHANISM` → `DEFAULT_ASSERTION_MECHANISM`. Report a uniform `mechanism.type = 'assertion'` for every violation and move the specific pragma to `mechanism.data.pragma`. Add a plugin↔runtime coupling test asserting the injected `CAPTURE_FN` matches the exported reporter name so a one-sided rename fails CI instead of breaking silently. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a CHANGELOG caveat that hard preconditions (invariant/assert) still throw after reporting — the transform yields a readable event, not crash suppression. Update the RN global handler comment to the assertions vocabulary. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review findings on the assertion instrumentation:
- assertion.ts: stringify values defensively so a Symbol (String(symbol)
throws) or a value with a throwing toString/Symbol.toPrimitive can't crash
the reporting path; fall back to [unstringifiable <type>].
- sentryAssertionBabelPlugin.ts: capture only identifiers whose binding is
visible from the call-site scope, so a nested arrow/callback param no longer
emits an out-of-scope reference (ReferenceError on the report path).
- sentryAssertionBabelPlugin.ts: normalize path separators before matching so
the SDK self-exclusion and the includeNodeModules allowlist work on Windows.
- Document that aliased/destructured pragmas (e.g. const { assert } = console)
are intentionally not matched.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Guard the reporter's `values` handling against `null` and throwing getters so the no-throw reporting path holds for hand-written calls. - Support `util.format`-style message interpolation via a new `messageArgs` option; the Babel plugin forwards variadic pragma args (e.g. RN's `invariant(dims, 'No dimension set for key %s', key)`). - Construct the call-site `Error` through a hoisted global-`Error` alias so a lexical `Error` shadow can't break the injected `new Error()`. - Normalize path separators in the transformer's node_modules gate so the `includeNodeModules` allowlist matches on Windows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Guard the `%d`/`%i`/`%f` coercions in the reporter's message formatter so a non-coercible arg (e.g. a Symbol) renders as `NaN` instead of throwing on the no-throw reporting path. - Drop `let`/`const` identifiers declared textually after the call site from the captured `values`, so a short-circuited condition can't read them in their temporal dead zone and throw a ReferenceError on the report path. - Only rewrite standalone assertion statements; a pragma used as a subexpression (e.g. `warning(cond) && next()`) is left untouched to preserve its semantics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…o__ keys, and @sentry-internal Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| for (const key of Object.keys(values)) { | ||
| try { | ||
| const value = values[key]; | ||
| data[`values.${key}`] = typeof value === 'boolean' ? value : truncate(stringifyValue(value), MAX_VALUE_LENGTH); | ||
| } catch (_e) { | ||
| // A throwing getter must not break the no-throw reporting path. | ||
| data[`values.${key}`] = '[unreadable]'; | ||
| } | ||
| } | ||
| try { | ||
| data.values = truncate(JSON.stringify(values) ?? 'undefined', MAX_SNAPSHOT_LENGTH); | ||
| } catch (_e) { | ||
| // Circular or non-serializable values — the flattened entries above still apply. |
There was a problem hiding this comment.
Unbounded values iteration in flattenValues allows DoS via large objects
flattenValues iterates over every key in values without a count cap and calls JSON.stringify(values) on the full object before truncation, allowing a large input object to exhaust CPU and memory.
Evidence
flattenValuescallsObject.keys(values)and loops over every key, creating onedataentry per key with no limit on the total number of entries.- It then calls
JSON.stringify(values)on the entire object before passing the result totruncate, so a large object is fully serialized first. - Individual values and snapshots are length-capped (
MAX_VALUE_LENGTHandMAX_SNAPSHOT_LENGTH), but the key count is unbounded, contradicting the function's JSDoc claim that a large object "can't bloat the event payload". - Because
captureAssertionViolationis a public API, a caller can pass an arbitrarily largevaluesobject (e.g. a config map or array with millions of indices), causing unbounded CPU and memory consumption on the no-throw reporting path.
Identified by Warden · find-bugs, code-review · CTS-DF4
There was a problem hiding this comment.
Fixed. flattenValues now caps the entry count at MAX_VALUE_ENTRIES (50) and builds the JSON snapshot from only that capped subset rather than JSON.stringify-ing the full input, so a large object handed to the public API no longer produces unbounded entries or forces a full serialization on the no-throw path. Over-cap keys are summarized in a values.__truncated__ marker, and the JSDoc is updated to describe all three bounds (count, per-entry length, snapshot length). Added a test with a 1000-key object (162eefb → 4ed8f2d).
…ptions Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| f9c1ed4+dirty | 431.00 ms | 466.22 ms | 35.22 ms |
| c2e182c+dirty | 471.64 ms | 553.59 ms | 81.95 ms |
| 41d6254+dirty | 424.45 ms | 474.34 ms | 49.89 ms |
| af33f3b+dirty | 434.90 ms | 506.14 ms | 71.24 ms |
| 882f8ae+dirty | 399.98 ms | 427.06 ms | 27.08 ms |
| 0b5120f+dirty | 503.22 ms | 538.60 ms | 35.38 ms |
| 774257e+dirty | 407.09 ms | 447.46 ms | 40.37 ms |
| bf168a4+dirty | 418.21 ms | 489.74 ms | 71.53 ms |
| 5569641+dirty | 406.43 ms | 428.51 ms | 22.08 ms |
| c151573+dirty | 530.34 ms | 559.43 ms | 29.09 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| f9c1ed4+dirty | 49.74 MiB | 54.86 MiB | 5.12 MiB |
| c2e182c+dirty | 49.74 MiB | 54.85 MiB | 5.11 MiB |
| 41d6254+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| af33f3b+dirty | 49.74 MiB | 55.09 MiB | 5.35 MiB |
| 882f8ae+dirty | 48.30 MiB | 53.60 MiB | 5.29 MiB |
| 0b5120f+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| 774257e+dirty | 48.30 MiB | 53.58 MiB | 5.28 MiB |
| bf168a4+dirty | 49.74 MiB | 55.09 MiB | 5.35 MiB |
| 5569641+dirty | 48.30 MiB | 53.48 MiB | 5.18 MiB |
| c151573+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7436d0f+dirty | 429.58 ms | 452.52 ms | 22.94 ms |
| 1122a96+dirty | 510.16 ms | 542.00 ms | 31.84 ms |
| 26843eb+dirty | 442.02 ms | 506.29 ms | 64.27 ms |
| 88735e9+dirty | 427.04 ms | 487.37 ms | 60.33 ms |
| 882f8ae+dirty | 408.19 ms | 435.86 ms | 27.67 ms |
| 1a2e7e0+dirty | 451.98 ms | 501.50 ms | 49.52 ms |
| 68672fc+dirty | 407.55 ms | 442.96 ms | 35.41 ms |
| 4966363+dirty | 415.67 ms | 448.60 ms | 32.93 ms |
| 038a6d7+dirty | 499.02 ms | 527.68 ms | 28.66 ms |
| 6176a94+dirty | 403.58 ms | 446.73 ms | 43.15 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7436d0f+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| 1122a96+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
| 26843eb+dirty | 49.74 MiB | 55.26 MiB | 5.52 MiB |
| 88735e9+dirty | 49.74 MiB | 54.82 MiB | 5.07 MiB |
| 882f8ae+dirty | 48.30 MiB | 53.60 MiB | 5.29 MiB |
| 1a2e7e0+dirty | 49.74 MiB | 54.82 MiB | 5.07 MiB |
| 68672fc+dirty | 48.30 MiB | 53.61 MiB | 5.31 MiB |
| 4966363+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
| 038a6d7+dirty | 48.30 MiB | 53.60 MiB | 5.30 MiB |
| 6176a94+dirty | 48.30 MiB | 53.54 MiB | 5.24 MiB |
- Guard the error.message backfill so a frozen/read-only/non-object error can't throw on the no-throw reporting path. - Match node_modules as a real path segment (not a raw substring) via a shared isInNodeModules() helper, so first-party files like node_modules_helper.ts are still instrumented. - Build siteId from a root-relative path with a parentDir/basename fallback, so files sharing a basename no longer collide in the runtime dedup set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
PII note — Unchecking the "No new PII / gated by This is a deliberate call for the experimental/hackweek landing — capturing the failing value is the core point of the feature, and gating it off-by-default would neuter it. If this graduates to an actual feature, revisit before release: likely "primitives always, full object/array snapshots behind |
| if (normalizedFilename.includes('node_modules')) { | ||
| if (!inc) { | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
First-party files with node_modules in path incorrectly skipped from assertion instrumentation
The includes('node_modules') substring check over-matches and skips first-party files like src/utils/node_modules_helper.ts, while the plugin itself correctly instruments them. Use the same path-segment regex the plugin uses.
Evidence
sentryAssertionBabelPlugin.ts:396uses/(?:^|\/)node_modules\//inisInNodeModules, explicitly documented to allow first-party files likesrc/utils/node_modules_helper.ts.addSentryCaptureAssertionsPluginusesnormalizedFilename.includes('node_modules')which matches any substring, not just a path segment.- A first-party file named
node_modules_helper.tsis skipped by the transformer guard but would be instrumented by the plugin, so the early return removes valid instrumentation. - The plugin's own test (
sentryAssertionBabelPlugin.test.ts:252-254) asserts thatnode_modules_helper.tsshould be instrumented.
Also found at 1 additional location
packages/core/src/js/tools/sentryAssertionBabelPlugin.ts:231-237
Identified by Warden · code-review, find-bugs · EPW-EC8
| /** | ||
| * Re-throw the `error` after reporting, preserving the original throwing | ||
| * semantics of hard preconditions (`invariant`, `assert`). The Babel transform | ||
| * sets this for pragmas listed in its `rethrowPragmas`, so downstream code that | ||
| * relied on the assertion halting execution is not reached with invalid state. | ||
| * | ||
| * The re-throw fires even when the report is deduplicated by `siteId` — | ||
| * deduplication suppresses the duplicate *event*, never the control flow. To | ||
| * avoid the rethrown error being reported a second time as an unhandled crash, | ||
| * the reporter tags it so Sentry's global handler skips it. | ||
| * | ||
| * Report-only pragmas (`warning`, `console.assert`) leave this `false`. | ||
| * | ||
| * @default false | ||
| */ | ||
| rethrow?: boolean; | ||
| } |
There was a problem hiding this comment.
Rethrown assertion events can be lost before transport flush
After captureException, rethrow: true tags the error and throws so the global handler skips capture—and also skips the production client.flush() that normally runs before defaultHandler tears the app down. Flush (or otherwise await outbound transport) on the already-captured path before invoking the default handler, otherwise invariant/assert violations often never leave the device.
Evidence
captureAssertionViolationqueues a handled event viacaptureException, thenrethrowCaptured()sets__sentry_captured__and rethrows (assertion.ts).setupErrorUtilsGlobalHandlerreturns early onerror?.__sentry_captured__with onlydefaultHandler(error, isFatal)— noclient.flush()(reactnativeerrorhandlers.ts).- The normal fatal path flushes up to
shutdownTimeout(default 2s) beforedefaultHandlerspecifically so events are not lost on teardown. - Default Babel rewrite emits
rethrow: trueforinvariant/assert, so this is the primary production path for hard preconditions.
Also found at 1 additional location
packages/core/src/js/integrations/reactnativeerrorhandlers.ts:171-174
Identified by Warden · code-review · SB2-FX7
| const uid = ensureHelperBinding(t, path, state); | ||
| const properties = buildReportProperties(t, path, state, filename, pragma, condition, options); |
There was a problem hiding this comment.
Eager helper require can cycle through RN modules and leave capture undefined
Hoist the helper as a lazy getter (or require a cycle-free subpath) so instrumented RN modules like Dimensions don't re-enter @sentry/react-native before captureAssertionViolation is exported.
Evidence
ensureHelperBinding()(called from the hunk at lines 644–645) injectsvar _capture… = require('@sentry/react-native').captureAssertionViolationat program top, so the require runs on module load.packages/core/src/js/index.tsexports./sdkbefore./assertion, andsdk.tsximportsFeedbackFormProvider, which importsDimensionsfromreact-native.- The sample Metro config allowlists
react-native/Libraries/Utilities/Dimensions, so loading that instrumented module re-enters the SDK entry whilecaptureAssertionViolationis still uninitialized and can bind the helper toundefined.
Identified by Warden · code-review · 26U-MTB
| function collectValueIdentifiers(conditionPath: NodePath<BabelTypes.Expression>): string[] { | ||
| const names = new Set<string>(); | ||
| // The `values` object is emitted in the call-site scope, so only capture | ||
| // identifiers that resolve to a binding visible there. An identifier bound in |
There was a problem hiding this comment.
Automatic condition value capture can send secrets to Sentry
Consider excluding sensitive identifier names or making values capture opt-in—failed asserts like invariant(apiKey) / assert(password.length >= 8) will attach live secrets to the event.
Evidence
collectValueIdentifiers()walks the assertion condition and records bound identifier names for emission in avaluesobject.- The rewrite only evaluates that object on the falsy path, then
captureAssertionViolation()flattens it intomechanism.data(values.<key>+ JSON snapshot). - No name denylist or opt-out exists, so identifiers such as
apiKey/password/tokenare sent on violation.
Identified by Warden · code-review · HK5-HXQ
| reportedSites.add(siteId); | ||
| } |
There was a problem hiding this comment.
siteId marked reported before capture can permanently drop a real violation
Move reportedSites.add(siteId) to after a successful captureException/eventId; otherwise a failed first attempt permanently suppresses later reports for that site.
Identified by Warden · find-bugs · GCZ-W7R
| function isNodeModulesExcluded(filename: string, includeNodeModules: boolean | string[] | undefined): boolean { | ||
| if (!isInNodeModules(filename)) { | ||
| return false; | ||
| } | ||
| if (!includeNodeModules) { | ||
| return true; | ||
| } | ||
| if (Array.isArray(includeNodeModules)) { | ||
| // Allowlist fragments are written with forward slashes; normalize so they | ||
| // still match on Windows (backslash) paths. | ||
| const normalized = toPosixPath(filename); | ||
| return !includeNodeModules.some(fragment => normalized.includes(fragment)); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Bare-string includeNodeModules instruments all node_modules
A non-array truthy includeNodeModules (e.g. 'react-native/...') falls through both guards and instruments every dependency; coerce non-arrays the same way asPragmaList does, or treat a string as a single allowlist entry.
Evidence
isNodeModulesExcludedonly special-cases falsy values andArray.isArray(...); any other truthy value returnsfalse(not excluded).asPragmaListalready exists for the same Metro-config mis-shape (pragmas/rethrowPragmasbare string → fallback), and its comment claims the same guard forincludeNodeModules.- Tests cover non-array
pragmas/rethrowPragmasbut not a bare-stringincludeNodeModules. addSentryCaptureAssertionsPlugininsentryBabelTransformerUtils.tsmirrors the same!inc/Array.isArray(inc)logic, so the plugin is still pushed for everynode_modulesfile whenincis a string.
Identified by Warden · find-bugs · Z49-ZGE
📢 Type of change
📜 Description
Adds an opt-in Metro/Babel transform that rewrites assertion call sites (
invariant,assert,warning,console.assert) so a violated assertion reports a non-fatal Sentry event instead of being stripped from release bundles or crashing with a minified message.captureAssertionsin the Sentry Metro config; off by default. First-party code is instrumented by default; dependencies are opt-in through anincludeNodeModulesallowlist.invariant/assert) still throw after reporting;warning/console.assertare report-only.captureAssertionViolation()(also usable by hand).mechanism.type = 'assertion'; the specific pragma is recorded undermechanism.data.pragma. Grouping keys by pragma + call site, so flavors stay separable without fragmenting the mechanism type.💡 Motivation and Context
Assertions are usually stripped from release builds or, when they do fire, surface as unreadable minified crashes. This surfaces them as grouped, non-fatal Sentry events with the condition, runtime values, and a stack anchored at the assertion site — without changing app source.
The public surface uses an "assertions" vocabulary (
captureAssertions,captureAssertionViolation,AssertionViolationOptions) rather than "invariant" so the naming isn't tied to a single pragma.💚 How did you test it?
Unit tests for the reporter, the Babel plugin, the transformer wiring, and the RN error handler — including a plugin↔runtime coupling test that fails CI if the injected helper name and the exported reporter name drift apart. Manually verified on the RN sample (iOS):
invariantfrom a dependency, first-partyconsole.assert, and a hand-written call each produced the expected handled events.📝 Checklist
sendDefaultPIIis enabled.🔮 Next steps
If this graduates from a hackweek experiment to a real feature, revisit before release:
valuesPII (primitives always; object/array snapshots behindsendDefaultPII).